feat: Cache Cover Art Calls #3015
Conversation
There was a problem hiding this comment.
I deployed to test.LB for in-situ testing, and it doesn't seem to be working for me.
I can see the stores in IndexedDB but it does not seem to store anything, the stores are empty, and going back and forth on two pages seems to call the CAA every time for all albums.
| storeName: "coverart", | ||
| }); | ||
|
|
||
| const coverArtCacheExpiry = localforage.createInstance({ |
There was a problem hiding this comment.
Is there an advantage to having a separate cache instead of storing an object with {expiry : xxxx, data: yyyyy} ?
Is it to make the homemade garbage collection more efficient?
I also saw that there was a plugin for handling expiry, and they implemented it using the same DB instance but adding a separate key-value pair, the key being based on the original key.
That could be another way to do it: https://github.com/LuudJanssen/localforage-cache/blob/master/src/localforage.js#L117-L124
There was a problem hiding this comment.
I believe, one of the important factors of saving data in IndexedDB / LocalStorage is the cleanup process. What removeAllExpiredCacheEntries does is run over all the keys and check if the key has been expired or not.
Saving the expiration key as ${key}_expires_${hash} makes this process slower as we'll have to index all keys, and filter down the keys of this regex and then process.
There was a problem hiding this comment.
interface CacheEntry {
data: string;
expiry: number
}
This approach is definitely better because a Single operation ensures data and expiry are always in sync, and makes the code simpler. BUT, when checking if data is expired, this forces you to deserialize the entire object which will increase memory usage and processing time.
I deployed the PR back to test, and I believe the PR is working fine. Did you try refreshing the DB again?
That's strange. I checked navigating through through the pages where release cards are being rendered, things are working fine. Can you share the network logs? |
|
Yes I had refreshed the DB, but still nothing. This time around testing, I do see two entries (for the same cover?) in there, but the other covers are still being loaded every time. The duplicate entry with true/false at the end that I have in IndexedDB, in case it is relevant: |
|
OK, so I see that the issue might be between my chair and my keyboard :) I was looking at my dashboard when testing, I didn't realize this was going to apply only to the release card. I tested again, and I'm still not sure it's working as expected.
For context, checking the indexedDB after all that, I see 13 entries only |
| @@ -756,13 +757,23 @@ const getAlbumArtFromReleaseGroupMBID = async ( | |||
| optionalSize?: CAAThumbnailSizes | |||
| ): Promise<string | undefined> => { | |||
| try { | |||
| const cacheKey = `rag:${releaseGroupMBID}-${optionalSize}`; | |||
| @@ -756,13 +757,23 @@ const getAlbumArtFromReleaseGroupMBID = async ( | |||
| optionalSize?: CAAThumbnailSizes | |||
| ): Promise<string | undefined> => { | |||
| try { | |||
| const cacheKey = `rag:${releaseGroupMBID}-${optionalSize}`; | |||
| const cachedCoverArt = await getCoverArtCache(cacheKey); | |||
There was a problem hiding this comment.
This needs to be applied to getAlbumArtFromListenMetadata somewhere as well, which is used by the ListenCard component.
This PR Implements a client-side caching system for cover art using IndexedDB (with LocalStorage fallback) to improve performance and reduce server load. The system includes automatic cache expiration and cleanup mechanisms.